Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Revolut] Support importing the fees #115

Merged
merged 5 commits into from
Aug 5, 2024
Merged

Conversation

alvarogarcia7
Copy link
Contributor

Support importing the fees from Revolut.

Based on #105 by @Dr-Nuke

Status:

  • Passes the pre-commit in my local setup.
  • Create tests for it. I don't know what's the best way to write tests here. Maybe you could please guide me?
  • Not breaking compatibility: The parameter is optional. Old clients should still work
  • Not breaking smart_importer. See below

Usage

When creating the Revolut Importer, pass an optional parameter fee:

revolut_importer = revolutimp.Importer(
  "data/revolut.*.csv",
  "Assets:Revolut:EUR", 
  "EUR", 
  fee={'account': "Expenses:Revolut:Fees"} # new attribute
)
CONFIG = [revolut_importer]

Smart Importer,

As discussed in #105, when the transaction has more than one posting, smart import won't work anymore.

Proposed workflow:

  1. Would it be possible to import once, without fee; then apply smart import.
  2. Then import again, with fee
  3. Then merge the transactions, given that:
  • The exports will be usually small (~10-100 entries)
  • The order is respected, hence the merge can be O(n), where n is the number of entries on the CSV file.

If you would be so kind to guide me along those lines, I'll be happy to implement such workflow.

@alvarogarcia7
Copy link
Contributor Author

As I kept investigating about this, I came up with an alternative:

A line of the Revolut CSV is turned into two transactions:

  1. Actual expense
  2. Revolut fee

an example:

2023-03-06 * "Cash ATM 3"
  Assets:Revolut:EUR                   -8.47 EUR
  Expenses:CashWithdrawal

2023-03-06 * "Fees for Cash ATM 3"
  Assets:Revolut:EUR                   -0.08 EUR
  Expenses:Revolut:Fees                 0.08 EUR 

This allows for the smart_importer to still work.

What are your thoughts, @tarioch ?

@tarioch
Copy link
Owner

tarioch commented Jul 29, 2024

I think the split keeps it simple to work with smart importer and I don't have a better idea, let's use that.

@tarioch
Copy link
Owner

tarioch commented Aug 1, 2024

@alvarogarcia7 do you want me to merge it like this or do you want to change it into the split transactions?

@alvarogarcia7
Copy link
Contributor Author

I have submitted the change in code, to add the fee as a new transaction.
thanks,

@alvarogarcia7
Copy link
Contributor Author

May I suggest to squash the changes, to actually lose the intermediate history?

@alvarogarcia7
Copy link
Contributor Author

Status:

  • Passes the pre-commit in my local setup.
  • Create tests for it. I don't know what's the best way to write tests here. Maybe you could please guide me?
  • Not breaking compatibility: The parameter is optional. Old clients should still work
  • Not breaking smart_importer. See below

@tarioch
Copy link
Owner

tarioch commented Aug 1, 2024

I think there is now a pre-commit failure with the latest changes, if you can fix that, I think we can merge it. Honestly I keep the tests a bit limited in here (I have some real data based tests for some of the importers in my personal repo). I think it's ok not to add a test for it.

@tarioch tarioch added the feature label Aug 5, 2024
@tarioch tarioch merged commit 4b3e52c into tarioch:master Aug 5, 2024
2 checks passed
This was referenced Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants